Skip to content

Add support for delete by value #103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

Bysmyyr
Copy link

@Bysmyyr Bysmyyr commented May 19, 2020

Add support for deleting by value. The behavior is based on this proposal: json-patch/json-patch2#18 and it is added with the default-true flag.

Should I apply this to v5 too?

@kszafran
Copy link

I suggest updating the README as well, so that it discusses this just like it does SupportNegativeIndices.

@Bysmyyr
Copy link
Author

Bysmyyr commented May 25, 2020

@evanphx Could you check this? Thanks!

@evanphx
Copy link
Owner

evanphx commented May 25, 2020

Is this in the RFC and I just missed it?

@kszafran
Copy link

kszafran commented May 25, 2020

@evanphx This is not part of the RFC. It is a part of the proposal found here: json-patch/json-patch2#18. We need this functionality ourselves, and since you already have some non standard features (negative indices) we thought that you might want to merge our changes, so that others also can benefit. If not, it's OK. We'll just keep using our fork. (I can explain why having the possibility to delete by value is very useful if you'd like.)

@evanphx
Copy link
Owner

evanphx commented May 25, 2020

I would prefer not not stray from the RFC any further because it is surprising for future package users. Especially if they upgraded and suddenly an API that didn't previously allow for deletion now did.

@evanphx evanphx closed this May 25, 2020
@Bysmyyr Bysmyyr deleted the patch branch May 26, 2020 06:11
@kszafran
Copy link

@evanphx I'm reapplying this change to v5 for our internal use. Seeing how json-patch now uses extended ApplyOptions, would you maybe reconsider merging this feature (I'd create a new PR)? I believe this simple feature is especially useful for REST APIs o avoid extra GET requests and race conditions, and so other users of this library could find it useful. Of course, if you still want to avoid further divergence from the RFC, that's understandable, and we'll just continue using a fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants